Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Rename nb_available to bytesavailable #25634

Merged
merged 1 commit into from
Jan 20, 2018
Merged

Conversation

ararslan
Copy link
Member

@ararslan ararslan commented Jan 19, 2018

nb_available is odd in that the prefix nb_ is pretty non-descriptive. Thus this PR renames the function to bytesavailable, which I think is more descriptive. Indeed, I find the following example from the diff especially pleasant:

eof(f::File) = bytesavailable(f) == 0

How do you know it's EOF? There are no bytes available. 🙂

@ararslan ararslan added io Involving the I/O subsystem: libuv, read, write, etc. deprecation This change introduces or involves a deprecation labels Jan 19, 2018
@ararslan ararslan mentioned this pull request Jan 19, 2018
19 tasks
@vchuravy
Copy link
Member

But why are there no bytesright? It parses slightly awkward to me, but that might just be my foreignspeakerness.
bytesremaining or bytesavailable seem more natural for me.

@Keno
Copy link
Member

Keno commented Jan 19, 2018

bytesleft also does not quite have the right connotations, since it's not the number of bytes left in the stream, rather it's the number of bytes available right now in the buffer.

@ararslan
Copy link
Member Author

How about bytesavailable then?

@vtjnash
Copy link
Member

vtjnash commented Jan 19, 2018

How about nbytesbuffered? (nb was non-blocking, but more clearly may have been nbuffered)

@bicycle1885
Copy link
Member

bytesavailable looks like returning buffered bytes. I think n prefix is needed here.

@ararslan
Copy link
Member Author

The PR now does bytesavailable. It has a nice symmetry with readavailable. I left off the n because nbytesavailable seems like quite a mouthful to me.

@ararslan ararslan changed the title Rename nb_available to bytesleft Rename nb_available to bytesavailable Jan 19, 2018
@StefanKarpinski
Copy link
Member

I like the name. I still dislike the function, but given that it exists, this is a good name.

@vtjnash
Copy link
Member

vtjnash commented Jan 19, 2018

I don't really like calling it "available" for the same reason I think the name "readavailable" is too confusing. These functions are defining that available means "buffered for instant, non-blocking access", but I think that seems hardly an intuitive definition.

@StefanKarpinski
Copy link
Member

but I think that seems hardly an intuitive definition.

What the heck else would it mean?

@vtjnash
Copy link
Member

vtjnash commented Jan 19, 2018

Bytes-transmitted (streams) or bytes-remaining (Files)

@StefanKarpinski StefanKarpinski merged commit 67ee6c6 into master Jan 20, 2018
@StefanKarpinski StefanKarpinski deleted the aa/nb_available branch January 20, 2018 16:17
@bicycle1885
Copy link
Member

I still think n prefixing is important here because it doesn't return buffered bytes themselves. n prefixing is a common practice in Base (e.g. ncodeunits, ndigits, ndims, nfields). I don't think just a bit mouthful name would not be a good reason to break the rule.

@ararslan
Copy link
Member Author

Fair point, @bicycle1885. I've opened #25659.

@stevengj
Copy link
Member

stevengj commented Feb 3, 2018

Would be good to have Compat support for bytesavailable...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
deprecation This change introduces or involves a deprecation io Involving the I/O subsystem: libuv, read, write, etc.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants